Getting rid of `instanceof`?

Here is my try. Consider defining an enum with the different Sprite types.

Here is my try. Consider defining an enum with the different Sprite types: class Sprite { public enum SpriteType { BOX, CAT, BOTTLE, HUMAN, TARGET, /* ... */, SIMPLE; } public SpriteType getSpriteType(){ return SIMPLE; } } class Box extends Sprite { @Override public SpriteType getSpriteType(){ return Sprite.SpriteType. BOX; } } And at last: public boolean isGameWon(Board board) { for (Point point : board.getTargetPoints()) if(board.

GetTopSpriteAt(point).getSpriteType()! =SpriteType. BOX) return false; return true; } This way, you can solve the problem of having to create an isATypeX() method in Sprite for every type X.

If you need a new type, you add a new value to the enum, and only the rule who need to check this type will need to aknowledge it.

1 Good try. This is nice because the enums play the role as semantic identifiers in the eyes of Rules, which in some way separates the semantics from the types. But does it really give anything to the code in terms of maintainability?

I'll need to think about this one for a bit :) – Thomas Ahle Jan 3 at 13:39 I think it will, as to create a new type only affects the enum, the type itself, and the rules to apply. – Tomas Narros Jan 3 at 14:36 1 That's still one more than just using the class type as the semantic type as well ;) – Thomas Ahle Jan 3 at 14:54 Yes, but without the instanceof: profit :D – Tomas Narros Jan 3 at 14:57 2 Hm, the enum system has the disadvantage compared to classes that it's harder to make hierarchies/groupings. – Thomas Ahle Jan 3 at 23:25.

Use polymorphism: class Sprite { .. someMethod(){ //do sprite } .. } class Box extends Sprite { .. @Overrides someMethod(){ //do box } .. } So, you just need to call sprite.someMethod() in your example.

I added an update to show why this won't work. – Thomas Ahle Jan 3 at 11:42 Please, show some code examples. – Artem Jan 3 at 11:48 How about now?

I don't see how someMethod() can be anything other than int isABox()? – Thomas Ahle Jan 3 at 11:51 How about cnt += sprite.size(); or getCount() or getSize() size returns 0 for most sprites and 1 for a Box. – Peter Lawrey Jan 3 at 11:55 You can incapsulate your cnt variable and logic related to it into some class and transfer instance of that class to someMethod().

– Artem Jan 3 at 11:56.

Basic overloading is the way to go here. It's the Sprite class hierarchy that should know what to do and how to do it, as in: interface Sprite { boolean isCountable(); } class MyOtherSprite implements Sprite { boolean isCountable() { return false; } } class Box implements Sprite { boolean isCountable() { return true; } } int count = 0; for (Point point : board.getTargetPoints()) { Sprite sprite = board. GetTopSpriteAt(point); count += sprite.isCountable()?1 : 0; } EDIT: Your edit to the question does not fundamentally change the problem.

What you have is some logic that is only applicable to Box. Again, encapsulate that particular logic in the Box instance (see above). You could go further and create a generic superclass for your sprites that defines a default value for isCountable() (note that the method is similar to the isBox one but is actually better from a design perspective, since it makes no sense for a Circle to have a isBox method - should Box also contain a isCircle method?

).

Basically, instead of if (sprite instanceof Box) // Do something Use sprite.doSomething() where doSomething() is defined in in Sprite and overriden in Box. If you want these rules separated from the Sprite class hierarchy, you can move them to a separate Rules class (or interface), where Sprite has a getRules() method and subclasses return different implementations. This would further increase flexibility, as it allows objects of the same Sprite subclass to have different behaviour.

– Thomas Ahle Jan 3 at 11:48 @Thomas Ahle: cnt += sprite.getTargetCount(), where getTargetCount() returns 1 in Box and 0 in ' Sprite`. There may be cleaner solutions, but that comes to mind immediately. And no, that's not at all the same as having an instanceof test in the code that does the counting.

– Michael Borgwardt Jan 3 at 12:00 I know where you want to go, but how would you describe that method in the docs? "Return 1 if you are a Box, 0 otherwise?". It might not be the same as "instanceof", but how is it any better?

– Thomas Ahle Jan 3 at 12:10 1 @Thomas Ahle: No, it should absolutely not be called that.It should apply to (and be named for) that specific behaviour of Boxes. The important thing is that the check now has a domain-specific name and is located in the type itself (or a rules class specific to that type). If you add a new class that behaves like a Box but should not be counted when on top of a Target, that class can have a different implementation of that one method.

– Michael Borgwardt Jan 3 at 12:13 @Thomas Ahle: "Return 1 if this object should count towards? If it's on top of a Target, 0 otherwise" – Michael Borgwardt Jan 3 at 12:15.

Here is an example of a generic counter without an isAnX() method for each type you might want to count. Say you want to count number of type X on the board. Public int count(Class type) { int count = 0; for (Point point : board.getTargetPoints()) if(type.

IsAssignable(board. GetTopSpriteAt(point))) count++; return count; } I suspect what you really want is public boolean isAllBoxes() { for (Point point : board.getTargetPoints()) if(!board. GetTopSpriteAt(point).isABox()) return false; return true; }.

Ah yes, that's much cleaner :) It still doesn't solve the problem of having to create an isATypeX() method in Sprite for every type X that I create (and the rules need to know of). – Thomas Ahle Jan 3 at 12:27 @ThomasAhle Perhaps the counting example I have given avoids the need for lots of isAnXxx() methods. – Peter Lawrey Jan 3 at 13:17 And then in my Rules I would have boolean isGameWon(Board board){return count(Box.

Class) == board.getTargetPoints();}? – Thomas Ahle Jan 3 at 13:32 You could but it would be less efficient as it will always count every Point even if the first is not a box. – Peter Lawrey Jan 3 at 14:52.

Therefore, I suggest these names: public boolean isGameWon(Board board) { for (Point point : board.getTargetPoints()) if(!board. GetTopSpriteAt(point).isWinningSprite()) return false; return true; } There is absolutely no point in having an isBox function. None whatsoever.

You might as well use instanceof. But if Box, Bottle and Target are all winning tiles, then you can have them all return class Box { public override bool isWinningSprite() { return true; } } You can then add another type of "winning" sprite without altering the isGameWon function.

1 ps I am not 100% about the isWinningSprite name, there may be a better name depending on what the game is you are making. – Chris Burt-Brown Jan 3 at 15:37 I agree with you, but actually this is what Tomas Narros answer is about too. The enum type "Box" could just as well be called the "winning sprite".

It's meaning is only in the eyes of the rules. What is important is to difference between class-types and rule-types. – Thomas Ahle Jan 3 at 15:55 @Thomas: If there is no possibility of any other sprites being a winning sprite, other than Boxes, then I believe that the cleanest, most maintainable solution might be to use instanceof.

– Chris Burt-Brown Jan 3 at 16:21.

Instanceof: (Almost) Always Harmful I took a look at all the answers to your post and tried to understand what you were doing. And I have come to the conclusion that instanceof is exactly what you want and your original code sample was fine. You clarified that: You are not violating the Liskov substitution principle since none of the Box code invalidates the Sprite code.

You are not forking the code with the response to instanceof. This is why people say instanceof is bad; because people do this: if(shape instanceof Circle) { area = Circle(shape).circleArea(); } else if(shape instanceof Square) { area = Square(shape).squareArea(); } else if(shape instanceof Triangle) { area = Triangle(shape).triangleArea(); } This is the reason why people avoid instanceof. But this is not what you are doing.

There is a one-to-one relationship between Box and winning the game (no other Sprites can win the game). So you are not in need of an additional "winner" sprite abstraction (because Boxes == Winners). You are simply checking the board to make sure that each top item is a Box.

This is exactly what instanceof is designed to do. Everyone else's answer (including my own) adds an additional mechanism for checking whether a Sprite is a Box. However they do not add any robustness.In fact you are taking features which are already supplied by the language and reimplementing them in your own code.

Tomas Narros argues that you should distinguish in your code between "semantic types" and "java types". I disagree. You have already established that you have a java type, Box, which subclasses Sprite.

So you already have all the information that you need. In my view, having a second independent mechanism which also reports "I am a Box", violates DRY (Don't Repeat Yourself). This means not having two independent sources for the same piece of information.

You now have to maintain an enum and a class structure. The so-called "benefit" is the ability to pirouette around a keyword which fully fills the purpose, and is harmful when used in more harmful ways. The golden rule is Use your head.

Don't obey rules as hard fact. Question them, learn why they are there, and bend them when appropriate.

Id=31 – Chris Burt-Brown Jan 3 at 16:55 1 Great answer. I really appreciate it. What I'm gonna do is try the suntax/semantics split and see if enough difference shows up no to violate DRY.

Or if I find myself confused about which kind of type to use over and over. – Thomas Ahle Jan 3 at 17:39.

General statements about object-oriented design / refactorings are hard to give IMHO as the "best" action depends on the context very much. You should try moving the "Do something" into a virtual method of Sprite which does nothing. This method can be called from within your loop.

Box can then override it and do the "something".

I think what people suggested to you is correct. Your doSomething() can look like this: class Sprite { public int doSomething(int cnt){ return cnt; } } class Box extends Sprite { @Override public int doSomething(int cnt){ return cnt + 1; } } So in your original code, you can do this: int cnt = 0; for (Point point : board.getTargetPoints()) { Sprite sprite = board. GetTopSpriteAt(point) cnt = sprite.

DoSomething(cnt); } Or else, you can also achieve the same goal with what you suggested but it may cost an additional calculation per loop. Class Sprite { public boolean isBox() { return false; } } class Box extends Sprite { @Override public boolean isBox(){ return true; } }.

But this is exactly the same as having a boolean isBox() in Sprite, which is exactly the same as having instanceof Box.. I suppose the problem here is in the preconditions where I want to know something about Boxes and Targets. This is syntax rather than semantics and violates that Liskov principle. I just don't know what else to do?

– Thomas Ahle Jan 3 at 11:57 Well, it's not exactly the same. If you override the method doSomething in Box, when you execute that method from any Sprite object which is a Box, it will trigger the method inside the Box class instead. You don't need an additional calculation for isBox() or instanceof.

Besides, you are not violating Liskov rule if you override the doSomething method. – Mr.J4mes Jan 3 at 12:04 Liskov substitution principle says that if S is a subtype of T, then objects of type T may be replaced with objects of type S without altering any of the desirable properties of that program (correctness, task performed, etc. ). The desirable properties of your program is to increment cnt if and only if the Sprite is a Box.

So if you substitute all your Sprite with its subclass and you still get the same desirable properties, you're not violating Liskov principle. – Mr.J4mes Jan 3 at 12:10 Ok, I see this might solve the problem on relying on specific implementations. But it would mean that for every new Sprite type I created, I would have to add a isNewType() method to Sprite and every other implementation.. – Thomas Ahle Jan 3 at 12:15 @ThomasAhle yeah.

Hence, I think it might be better if you override doSomething. – Mr.J4mes Jan 3 at 12:25.

That's what getTopSpriteAt does. Takes something from the top of the stack. I just added it there, because it might be useful in a pattern matching solution.

– Thomas Ahle Jan 3 at 11:43 Oops, my fault. Well, I suggest going with @Artems answer. – DerMike Jan 3 at 11:47.

I cant really gove you an answer,but what I can give you is a way to a solution, that is you have to find the anglde that you relate to or peaks your interest. A good paper is one that people get drawn into because it reaches them ln some way.As for me WW11 to me, I think of the holocaust and the effect it had on the survivors, their families and those who stood by and did nothing until it was too late.